Skip to content

Postgraphile v5 Support (#58) - #66

Open
dargmuesli wants to merge 26 commits into
graphile:mainfrom
dargmuesli:fzy/v5/dargmuesli
Open

Postgraphile v5 Support (#58)#66
dargmuesli wants to merge 26 commits into
graphile:mainfrom
dargmuesli:fzy/v5/dargmuesli

Conversation

@dargmuesli

Copy link
Copy Markdown

Description

Further work on the v5 migration started by @FelixZY in #60.
Resolves #58

Performance impact

unknown

Security impact

unknown

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

FelixZY and others added 14 commits June 3, 2024 14:33
After bumping to the v5 package versions, tests would fail with the
following error:

```
● Test suite failed to run

  ReferenceError: TextEncoder is not defined

  > 1 | import * as pg from "pg";
      | ^
    2 |
    3 | export async function withPgPool<T = any>(
    4 |   cb: (pool: pg.Pool) => Promise<T>

    at Object.<anonymous> (node_modules/pg/lib/crypto/utils-webcrypto.js:22:21)
    at Object.<anonymous> (node_modules/pg/lib/crypto/utils.js:8:20)
    at Object.<anonymous> (node_modules/pg/lib/crypto/sasl.js:2:16)
    at Object.<anonymous> (node_modules/pg/lib/client.js:5:12)
    at Object.<anonymous> (node_modules/pg/lib/index.js:3:14)
    at Object.<anonymous> (__tests__/helpers.ts:1:1)
    at Object.<anonymous> (__tests__/schema.minimal_type.test.ts:2:1)
```

Based on [information from @SimenB](
  jsdom/jsdom#2524 (comment)
), it seems like you probably should not be using
`jest-environment-jsdom` to start with if you need access to
`TextEncoder` or `TextDecoder`.

Based on [this](
  https://stackoverflow.com/a/72369912/1137077
) answer on SO, I was able to verify that `@jest-environment node` at
the top of test files fixed the issue. However, it would seem more
logical to apply this as a global setting, given that postgraphile is
meant to run in a node context.

After setting `testEnvironment: jest-environment-node` in the global
config, I found that tests again started failing. However, the new
failures seem related to the v5 changes to plugins which is expected at
this stage.
The previous `moduleResolution: node` setting prevent importing types
from `graphile-build-pg/pg-introspection`.

`NodeNext` was chosen based on
https://github.com/graphile/crystal/blob/91e87ab6516490a4cc7b7fc6400efb7623fbd331/graphile-build/graphile-build/tsconfig.json#L9
This seems to be better in line with other v5 plugins and the new
`GraphileConfig.Preset` type.
@socket-security

socket-security Bot commented Feb 15, 2026

Copy link
Copy Markdown

@socket-security

socket-security Bot commented Feb 15, 2026

Copy link
Copy Markdown

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn Critical
Critical CVE: Handlebars.js has JavaScript Injection via AST Type Confusion

CVE: GHSA-2w6w-674q-4c4q Handlebars.js has JavaScript Injection via AST Type Confusion (CRITICAL)

Affected versions: >= 4.0.0 < 4.7.9

Patched version: 4.7.9

From: yarn.locknpm/ts-jest@29.4.6npm/handlebars@4.7.8

ℹ Read more on: This package | This alert | What is a critical CVE?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/handlebars@4.7.8. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

Comment thread src/index.ts Outdated
Comment thread src/interfaces.ts Outdated
Comment thread src/Postgis_GeometryCollection_GeometriesPlugin.ts Outdated
Comment thread src/Postgis_Point_LatitudeLongitudePlugin.ts Outdated
Comment thread src/PostgisRegisterTypesPlugin.ts Outdated
Comment thread src/PostgisRegisterTypesPlugin.ts Outdated
Comment on lines +93 to +94
(attribute.extensions as any).postgisTypeModifier =
pgAttribute.atttypmod;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm as yet undecided on the best way to handle type modifiers in V5.

@dargmuesli dargmuesli Jul 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prototyped this against benjaie/crystal variants branch (graphile/crystal#3109) locally: built it and linked @dataplan/pg / graphile-build-pg (and their shared deps: grafast, graphile-build, graphile-config, pg-sql2, pg-introspection, graphql) into node_modules via symlinks, per your suggestion.

Reworked PostgisRegisterTypesPlugin to build the modified geometry/geography codecs in pgCodecs_findModifiedPgCodec (decoding the PostGIS typmod into subtype/Z/M, baseCodec: event.baseCodec), then map each one to its narrowed GraphQL type via setGraphQLTypeForPgCodec once the interfaces/object types are registered. That let me delete the manual per-field type lookup from PostgisColumnsPlugin; it now only overrides the SQL-computing plan, and gets its output type for free from core's normal codec→type resolution.

Results:

  • All 65 existing tests pass unchanged.
  • Since narrowing now lives on the codec instead of being recomputed per-attribute, it also correctly narrows geometry(...)-typed view columns (spot-checked manually) with no extra code.
  • Side note: function return types don't carry a typmod in pg_catalog at all, so that case isn't fixable on our end regardless of approach.

Prototype is on prototype/crystal-3109-codec-level-typmod (not part of this PR yet, since it depends on the unmerged pgCodecs_findModifiedPgCodec/baseCodec API and would silently regress the current, working attribute-level narrowing if run against the published graphile-build-pg).

Happy to open this as the real change the moment #3109 ships! Let me know if the shape looks right in the meantime.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @TBA-Lucas

I've also fixed mutations and JSON geography inserts. My prod application works fine now 🙌

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for pushing this forward!

Comment thread src/version.ts Outdated
Comment thread src/index.ts
Comment thread README.md Outdated
Comment thread tsconfig.json Outdated

@benjie benjie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a thorough review; but it's looking good! Try and minimize the difference with previous code as that makes review easier (and also eases migration for people who may have custom inflectors or other integrations/extensions). Thanks for your work on this!

@dargmuesli
dargmuesli requested a review from benjie February 16, 2026 19:30

@dargmuesli dargmuesli left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying out real world use of this at the moment. There are still things to discuss for sure that I'm not certain about yet. I was a (pure) consumer of these things before and just relied on them working, now this is me looking into their core which I always like to do when I have the time. Now there is the missing 20% or 10% maybe that I would spend very much time on compared to its result so maybe I should not do this. But I'd definitely collaborate on this further! If more brains share how to get this done, I'll keep this PR up to speed.

Comment thread __tests__/integration/__snapshots__/queries.test.ts.snap Outdated
@dargmuesli

Copy link
Copy Markdown
Author

@benjie I think this is ready for a full review now from my side, I have this running for some time in my dev setup now and outputs seem to fit!

@TBA-Lucas

Copy link
Copy Markdown

I tested the PR but I have issues with geometry(geometry, 4326) columns from Postgis returning "geom": null in postgraphile v5.

I only get GeometryGeometry as subtype, it seems that the plugin is currently unable to infer the specific subtype like GeometryPoint. Since our postgraphile v4 server is doing that just fine with the same database, I assume that due to migrating it from postgraphile v4 to v5 the PostGIS plugin is now stricter about how it decides geometry subtypes?

@dargmuesli

Copy link
Copy Markdown
Author

I noticed an issue writing to the DB too, reading worked fine for my use case. I'll look into it shortly.

npm/pnpm run the prepare lifecycle script (not prepack) when installing
a package directly from a git repository, since published tarballs
already ship a built dist/ but git checkouts don't. Without this,
depending on this branch directly (e.g. via a git URL) installs a
package with no dist/index.js.
…raphy columns

Two bugs found while investigating a reported GeoJSON input error:

1. PostgisColumnsPlugin's field plan assumed every record was a
   PgSelectSingleStep (which has `.select()`), but insert/update/delete
   mutation steps (PgInsertSingleStep/PgUpdateSingleStep/PgDeleteSingleStep)
   don't have that method - only `.get()`. So reading a geometry/geography
   field back from any create/update/delete mutation payload threw
   "$record.select is not a function". Fixed by branching on step type:
   `.select()` for queries (unchanged), and for mutations, build the same
   SQL expression via `pgClassExpression()` referencing the row's own
   `.alias` directly - the same approach these steps' own `.get()` uses for
   plain attributes.

2. The base geometry/geography codec's `toPg` was an identity function, so
   an incoming GeoJSON object (from the `GeoJSON` input scalar) was sent to
   Postgres as a JSON.stringify'd string. This happened to "work" for
   `geometry` columns only because `geometry_in` undocumentedly
   lenient-parses GeoJSON text - `geography_in` does not, and fails hard
   with "parse error - invalid geometry" (this was the actual server-side
   failure behind a mutation that looked, in GraphiQL/Ruru's variable
   editor, like a client-side "GeoJSON expects string/number/boolean"
   linter false positive - that lint warning is real but unrelated; this
   parse error is what actually broke the request). Added
   src/geoJsonToWkt.ts to convert GeoJSON to WKT/EWKT explicitly in `toPg`,
   which both `geometry_in` and `geography_in` accept identically -
   verified against Point, 3D Point (Z), LineString, Polygon, MultiPolygon,
   and GeometryCollection round-tripped through real create mutations.

All 65 existing tests still pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Postgraphile V5 Support

4 participants